Skip to content

fix(browser): avoid attachment menu coordinate clicks - #431

Open
oraclexing wants to merge 1 commit into
steipete:mainfrom
oraclexing:fix/browser-attachment-plus-safety
Open

fix(browser): avoid attachment menu coordinate clicks#431
oraclexing wants to merge 1 commit into
steipete:mainfrom
oraclexing:fix/browser-attachment-plus-safety

Conversation

@oraclexing

@oraclexing oraclexing commented Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

  • activate ChatGPT's attachment menu through the exact composer + control
  • replace trusted page-coordinate clicks with focused trusted keyboard activation
  • abort before prompt submission if activation enters Work or another conversation, including project-scoped /g/.../c/<id> routes
  • document and test the Work-home attachment regression path

Why

#315 / #316 normalize ChatGPT Work to Chat during startup, but attachment upload later used broad fallback selectors plus a trusted mouse click at a computed page coordinate. On the current Work home, suggestion cards sit close to the composer.

In a sanitized live failure, an attachment-bearing Oracle run had promptSubmitted: false; an unrelated Work suggestion became a new task about 18 seconds after the run began, and Oracle then failed one attachment-readiness timeout later. No separate browser-control call occurred in the invoking turn. The timing strongly implicates attachment-menu activation, although the exact misdirected pointer event was not captured.

This patch removes that coordinate-click hazard. It targets only #composer-plus-btn / button[data-testid="composer-plus-btn"], focuses the control, sends trusted Enter key events, and keeps only an exact-selector synthetic fallback. A post-activation guard rejects Work or unexpected conversation navigation.

Validation

Exact head: 7d5a05233b0894bb551f75140fa34434e8400886 (rebased onto current main at 40c188caa861017ba48e7b496e2171317c648976).

  • pnpm exec vitest run tests/browser/pageActions.test.ts — 101 passed, 1 skipped
  • pnpm run lint — passed
  • pnpm run build — passed
  • regression coverage proves unexpected navigation is rejected for both root /c/<id> and project-scoped /g/.../c/<id> routes with workSelected: false

Signed-in browser smoke

A compiled exact-head overlay ran an Instant GPT-5.6 Sol consult with one uploaded LICENSE file:

  • session: attachment-safety-final-smoke-20260828
  • attachment UI became ready and the prompt submitted to an ordinary /c/... conversation
  • answer: ORACLE_ATTACHMENT_SAFETY_FINAL_OK
  • completed in 39.2s and archived the smoke conversation
  • post-run sidebar inspection still showed only the two pre-existing Work entries; no new Work task appeared

After restarting Codex, the installed MCP overlay was also verified through detached consult plus a single blocking wait:

  • session: oracle-mcp-attachment-restart-smoke
  • answer: ORACLE_MCP_ATTACHMENT_RESTART_OK
  • completed with one real attachment and archived successfully
  • post-run sidebar inspection again showed no new Work task

An explicit navigation to an existing Work conversation also failed closed before attachment upload or prompt submission.

@clawsweeper

clawsweeper Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 28, 2026
@clawsweeper

clawsweeper Bot commented Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 9:59 PM ET / September 2, 2026, 01:59 UTC.

ClawSweeper review

What this changes

The PR replaces coordinate-based ChatGPT attachment-menu and attachment-send clicks with exact controls, keyboard activation, and page-identity guards.

Regression provenance

Possible regression — probable (reviewed change; failure trace). No predecessor PR is attributed.

Merge readiness

⛔ Blocked until stronger real behavior proof is added - 3 items remain

Keep open: the source change resolves the prior final-dispatch finding, but the supplied signed-in proof names an earlier head rather than the current head, so this exact branch still needs real behavior evidence before merge.

Priority: P1
Reviewed head: 13b4d5f8217807cf77b5ab85904db69aee9dada3

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The patch is focused and covers the prior blocker, but current-head real browser proof is still missing.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: The body describes redacted signed-in attachment runs and a Work fail-closed case, but it explicitly identifies 7d5a052 rather than the reviewed 13b4d5f head that adds the final-dispatch guard; provide a redacted current-head trace showing both ordinary commit and late-navigation refusal. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The body describes redacted signed-in attachment runs and a Work fail-closed case, but it explicitly identifies 7d5a052 rather than the reviewed 13b4d5f head that adds the final-dispatch guard; provide a redacted current-head trace showing both ordinary commit and late-navigation refusal. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 5 items Final-dispatch guard: Attachment sends focus only the exact send button, recheck the preserved navigation identity immediately before Enter, and fail before dispatch when Work or a different conversation is detected.
Focused delayed-navigation coverage: The current test exercises delayed navigation after attachment readiness and asserts that no keyboard or mouse send event occurs.
Current-head proof gap: The PR body identifies signed-in smoke evidence for 7d5a052, while the reviewed head is 13b4d5f; the supplied live evidence therefore does not prove the current final-dispatch implementation.
Findings None None.
Security None None.

How this fits together

Oracle’s browser mode drives a signed-in ChatGPT composer to upload files and submit consultations. This change controls attachment preparation and final send steps before a ChatGPT conversation turn is committed.

flowchart LR
A[Oracle browser consultation] --> B[Capture Chat page identity]
B --> C[Open exact attachment menu]
C --> D[Upload and verify files]
D --> E[Recheck page identity]
E --> F[Activate exact send button]
F --> G[Committed ChatGPT turn]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The body describes redacted signed-in attachment runs and a Work fail-closed case, but it explicitly identifies 7d5a052 rather than the reviewed 13b4d5f head that adds the final-dispatch guard; provide a redacted current-head trace showing both ordinary commit and late-navigation refusal. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - The current head adds the final navigation recheck, but the supplied signed-in runs identify the preceding head; without an exact-head trace, attachment delivery and fail-closed behavior remain unproven in the real ChatGPT UI.
  • Complete next step (P2) - The remaining merge blocker is contributor-specific real behavior proof, not a repair suitable for an automated fix lane.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +304 net lines, tests +415 lines The focused browser-send behavior is accompanied by more targeted regression coverage than production growth.

Merge-risk options

Maintainer options:

  1. Refresh exact-head browser proof (recommended)
    Provide a redacted current-head signed-in attachment run plus a delayed-navigation refusal trace before merging.

Technical review

Best possible solution:

Add a redacted signed-in trace from the current head showing one attachment committed to the intended Chat conversation and a delayed Work or conversation navigation rejected before any send input.

Do we have a high-confidence way to reproduce the issue?

Yes: current source and focused coverage reproduce the delayed-navigation condition at the final attachment dispatch boundary, though this review did not execute the signed-in browser path.

Is this the best way to solve the issue?

Yes in design: preserving identity and rechecking it directly before the exact keyboard send is the narrowest fix for the reported misdirected activation; exact-head live proof remains required.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 40c188caa861.

Labels

Label changes:

  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body describes redacted signed-in attachment runs and a Work fail-closed case, but it explicitly identifies 7d5a052 rather than the reviewed 13b4d5f head that adds the final-dispatch guard; provide a redacted current-head trace showing both ordinary commit and late-navigation refusal. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove proof: sufficient: Current real behavior proof status is insufficient, not sufficient.
  • remove status: ⏳ waiting on author: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P1: The PR changes the attachment submission path, where a failure can prevent a browser consultation from reaching the intended chat.
  • merge-risk: 🚨 message-delivery: The new navigation checks and exact-button dispatch directly determine whether an attachment prompt is delivered.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body describes redacted signed-in attachment runs and a Work fail-closed case, but it explicitly identifies 7d5a052 rather than the reviewed 13b4d5f head that adds the final-dispatch guard; provide a redacted current-head trace showing both ordinary commit and late-navigation refusal. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Final-dispatch guard: Attachment sends focus only the exact send button, recheck the preserved navigation identity immediately before Enter, and fail before dispatch when Work or a different conversation is detected. (src/browser/actions/promptComposer.ts:769, 13b4d5f82178)
  • Focused delayed-navigation coverage: The current test exercises delayed navigation after attachment readiness and asserts that no keyboard or mouse send event occurs. (tests/browser/promptComposer.test.ts:433, 13b4d5f82178)
  • Current-head proof gap: The PR body identifies signed-in smoke evidence for 7d5a052, while the reviewed head is 13b4d5f; the supplied live evidence therefore does not prove the current final-dispatch implementation. (13b4d5f82178)
  • Attachment-flow history: The prior mainline attachment-send implementation was maintained in the recent attachment hardening commit, which is an ancestor of the reviewed head. (src/browser/actions/attachments.ts, bbc1b3b0261d)
  • Release status: The unmerged reviewed head is not contained in a local release tag, so this is not already-shipped behavior. (13b4d5f82178)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • DragonFSKY: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Post a redacted current-head signed-in trace for a successful attachment send and a delayed Work or conversation navigation that is rejected before send input.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-08-28T13:26:55.923Z sha 7b753df :: needs changes before merge. :: [P1] Handle project-scoped conversation URLs in the navigation guard
  • reviewed 2026-09-02T01:17:56.140Z sha 7d5a052 :: needs changes before merge. :: [P1] Recheck navigation immediately before prompt submission

@oraclexing
oraclexing force-pushed the fix/browser-attachment-plus-safety branch from 7b753df to 7d5a052 Compare September 2, 2026 01:12
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Sep 2, 2026
@oraclexing
oraclexing force-pushed the fix/browser-attachment-plus-safety branch from 7d5a052 to 13b4d5f Compare September 2, 2026 01:55
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient Contributor real behavior proof is sufficient. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@oraclexing